Fix enforce rekey read and time limits - #766
Open
inureyes wants to merge 3 commits into
Open
Conversation
…ation `Limits` exposes `rekey_read_limit` and `rekey_time_limit`, but `Encrypted::flush` is only reached on the write path, so neither limit could fire. A session that mostly downloads, or one that idles, kept the same keys indefinitely no matter how the limits were configured. `rekey_write_limit` also counted the cleartext KEX packets of the current epoch, because `SSHBuffer::bytes` was never reset when a new cipher was installed. Count inbound payload bytes in `cipher::read`, reset the write counter in `PacketWriter::set_cipher` so each key epoch starts at zero, and arm a rekey deadline in the client and server event loops so an idle session still rekeys on time. Gate automatic rekeying on completed authentication in both loops. Otherwise a small `RekeyLimit` makes the pre-authentication traffic start a second key exchange while the first one is still being set up. On the server, `USERAUTH_SUCCESS` is flushed while the state is still `InitCompression`, so activate deferred server compression before emitting `KEXINIT` when rekeying is the first post-authentication write. Explicit `Session::initiate_rekey` and the `Msg::Rekey` path are unchanged and still rekey on demand.
inureyes
changed the title
`#[tokio::test]` is a proc macro, so unlike the built-in `#[test]` it expands even when the crate is not compiled in test mode. `sshbuffer.rs` keeps its tests at module top level rather than in a `#[cfg(test)] mod`, so the new async test broke `cargo build --target wasm32-wasip1 --no-default-features --features flate2,ring` with "The #[tokio::test] macro requires rt or rt-multi-thread". Gate that one test on `cfg(test)` so the attribute is stripped before expansion. Verified with the CI command on 1.89.0 against `wasm32-wasip1`, and `cargo test -p russh --lib` still runs all 195 tests including this one.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Limitsexposesrekey_read_limitandrekey_time_limit, but the only place that consults them isEncrypted::flush, which is reached on the write path. A session that mostly downloads, or one that idles, keeps the same keys no matter how the limits are configured.rekey_write_limitis also off by the current epoch's cleartext KEX packets, becauseSSHBuffer::bytesis never reset when a cipher is installed.cipher::readcounts inbound payload bytes, and the client and server loops compare that count againstrekey_read_limit, resetting it when a key exchange completes.PacketWriter::set_cipherresets the write counter so each key epoch starts at zero.rekey_time_limitandEncrypted::last_rekey, so an idle session still rekeys on time.RekeyLimitmakes pre-authentication traffic start a second key exchange while the first is still being set up. On the server,USERAUTH_SUCCESSis flushed while the state is stillInitCompression, so deferred server compression is activated beforeKEXINITwhen rekeying is the first post-authentication write.Session::initiate_rekeyandMsg::Rekeyare unchanged and still rekey on demand.Ten tests cover the read limit, the deadline, the pre- and post-authentication gate on both sides, and the epoch reset. Dropping the byte accounting makes the two
sshbuffertests fail.compression::tests::partial_flush_packets_round_tripfails onmainat d3ae702 as well, and #757 looks like the fix. Everything else passes....
AI Usage
Choose the level of AI involvement for this PR.
This is not to block AI contributions but rather to speed up PR review (saves time on trying to deduce the logic behind AI hallucinations).